-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
const-eval: always do full typed copies #149901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@bors try |
This comment has been minimized.
This comment has been minimized.
const-eval: always do full typed copies
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (db4e1d2): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.9%, secondary -1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.4%, secondary 261.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 473.727s -> 474.872s (0.24%) |
|
While we're here... @craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
The regressions.... look genuine? |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Yeah it's not too surprising that some const code out there has UB. I am honestly surprised the number is so small, that's a good sign IMO. :) |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
I looked at all the regressions. The service-install crate has a "compile error -> ICE" regression which is a nondeterministic ICE, and this exact same crate has had this nondeterministic ICE before. See #147966 (comment) Other than that, all regressions involved code doing UB in const that was previously undetected. Regressed dependenciesbaby_shark 0.3.3: Used cglue 0.2.14: Creates dangling reference which was never dereferenced. Also does shenanigans with layout of RawWaker. Seems to be fixed in a newer version of igneous-labs/inf-1.5 and igneous-labs/solido-legacy-sdk: Depends on Regressed root resultsLohann/merkle-patricia-trie-demo: Created a dangling reference before coercing to a raw pointer https://github.com/Lohann/merkle-patricia-trie-demo/blob/e5202c18669ac4cba1de29ec7f24d3720217901f/rust/merkle-patricia-trie-js/src/storage.rs#L15-L16 TadaHrd/AES-rust: Code copied from wyhhh/snake: Transmuted a aes_rust: Used const_crypto: Used const_lookup_map: Used igneous-labs/solido-legacy-sdk: Already mentioned as regressed dependency lunka: Used mesh_to_vox: Used vptr: Used a |
Perf experiment exploring alternatives to #148967.
This is the maximally naive version that just always does full typed copies. It may be possible to skip some of the validation work if all we care about is resetting padding -- though it's not obvious to me how to do that in a way that actually helps with performance. Also, typed copies at integer type technically do not preserve provenance, which could become relevant in const-eval for the same reason as provenance in padding (see #148470) -- so it's not like we can just skip all types without padding.